Limit impact of `typeof()` in a custom attribute #620

MichalStrehovsky · 2021-02-01T14:55:32Z

When a type is referenced by a typeof in a custom attribute, don't immediately create a constructed EEType for it.

This is a heuristic - we don't know what the type will be used for. The field/property/constructor parameter that is initialized with the typeof should ideally have a dataflow annotation that describes the intent - the dataflow infrastructure will then make sure we have all that's needed.

I hit this while I was working on #619 - the stubbed out method had a [AsyncStateMachine(typeof(<GetNonFileStreamAsync>d__2))] attribute on it. The code I'm making conditional was making a constructed type for the state machine. Then #618 created a perfect storm that brought a constructed HttpClient object back into the picture and rendered #619 completely ineffective by itself.

When a type is referenced by a typeof in a custom attribute, don't immediately create a constructed EEType for it. This is a heuristic - we don't know what the type will be used for. The field/property/constructor parameter that is initialized with the `typeof` should ideally have a dataflow annotation that describes the intent - this is then going to root he appropriated structures.

jkotas · 2021-02-01T15:40:14Z

the dataflow infrastructure will then make sure we have all that's needed.

How is the dataflow infrastructure going to do this?

MichalStrehovsky · 2021-02-01T15:43:50Z

class FooAttribute : Attribute
{
    [DynamicallAccessedMembers(Constructors)]
    public Type Foo;
}

[Foo(Foo = typeof(OtherClass))]
class SomeClass
{
}

class OtherClass
{
}

Dataflow analysis considers this an assignment of OtherClass into Foo. It will make sure we keep all constructors on OtherClass. This also keeps the owning type. Also makes it possible to Activator.CreateInstance whatever we get out of Foo without warnings.

jkotas · 2021-02-02T01:10:03Z

...t/ILCompiler.Compiler/Compiler/DependencyAnalysis/CustomAttributeBasedDependencyAlgorithm.cs

+            if (typeofType.IsArray
+                || (typeofType.HasInstantiation && !typeofType.IsGenericDefinition))
+            {
+                // We go for the entire EEType because the reflection stack at runtime will need an EEType anyway


Is there a good place in the reflection stack that shows the dependency on EEType and that can be linked from here?

It looks odd to me that the reflection stack does not need EETypes for regular types, but it does need them for arrays and generics.

IIRC is this one for arrays:

runtimelab/src/coreclr/nativeaot/System.Private.Reflection.Core/src/System/Reflection/Runtime/General/TypeUnifier.cs

Lines 278 to 289 in 2c62504

private static void ValidateElementType(RuntimeTypeInfo elementType, RuntimeTypeHandle typeHandle, bool multiDim, int rank)

{

Debug.Assert(multiDim || rank == 1);

if (elementType.IsByRef)

throw new TypeLoadException(SR.Format(SR.ArgumentException_InvalidArrayElementType, elementType));

// We only permit creating parameterized types if the pay-for-play policy specifically allows them *or* if the result

// type would be an open type.

if (typeHandle.IsNull() && !elementType.ContainsGenericParameters && !(elementType is RuntimeCLSIDTypeInfo))

throw ReflectionCoreExecution.ExecutionDomain.CreateMissingArrayTypeException(elementType, multiDim, rank);

}

And this one for generic types:

runtimelab/src/coreclr/nativeaot/System.Private.Reflection.Core/src/System/Reflection/Runtime/General/TypeUnifier.cs

Lines 426 to 443 in 2c62504

protected sealed override RuntimeConstructedGenericTypeInfo Factory(UnificationKey key)

{

bool atLeastOneOpenType = false;

foreach (RuntimeTypeInfo genericTypeArgument in key.GenericTypeArguments)

{

if (genericTypeArgument.IsByRef || genericTypeArgument.IsGenericTypeDefinition)

throw new ArgumentException(SR.Format(SR.ArgumentException_InvalidTypeArgument, genericTypeArgument));

if (genericTypeArgument.ContainsGenericParameters)

atLeastOneOpenType = true;

}

// We only permit creating parameterized types if the pay-for-play policy specifically allows them *or* if the result

// type would be an open type.

if (key.TypeHandle.IsNull() && !atLeastOneOpenType)

throw ReflectionCoreExecution.ExecutionDomain.CreateMissingConstructedGenericTypeException(key.GenericTypeDefinition, key.GenericTypeArguments.CloneTypeArray());

return new RuntimeConstructedGenericTypeInfo(key);

}

(The // We only permit part)

If we just go and delete those lines things will probably still work.

The intent of that code was to blame MakeGenericType/MakeArrayType for reflection failures at runtime instead of having these APIs succeed and getting an exception once we actually want to do something with the type (call static method, activate, etc.).

I think this behavior makes sense for user code because this API is the problem.

It's annoying for framework code because e.g. without the special casing in the compiler we will get an exception when trying to GetCustomAttributeData for the attribute.

On a second thought, maybe the compiler doesn't need to do this special casing here. This situation is the same as what we would get when e.g. querying fields:

using System; ClassWithField.Klass = new ClassWithField() { MyGen = null }; typeof(Gen<>).ToString(); foreach (var f in typeof(ClassWithField).GetFields(System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.Public)) { Console.WriteLine(f.FieldType.FullName); } class ClassWithField { public static ClassWithField Klass; public Gen<int> MyGen; } class Gen<T> { }

This will fail with:

Unhandled Exception: System.Reflection.MissingMetadataException: 'Gen<System.Int32>' is missing metadata. For more information, please visit http://go.microsoft.com/fwlink/?LinkID=392859 at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.ConstructedGenericTypeTable.Factory(RuntimeConstructedGenericTypeInfo.UnificationKey) + 0x24b at System.Collections.Concurrent.ConcurrentUnifierWKeyed`2.GetOrAdd(K) + 0x23f at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.GetRuntimeConstructedGenericTypeInfo(RuntimeTypeInfo, RuntimeTypeInfo[], RuntimeTypeHandle) + 0x6f at System.Reflection.Runtime.TypeInfos.RuntimeConstructedGenericTypeInfo.GetRuntimeConstructedGenericTypeInfo(RuntimeTypeInfo, RuntimeTypeInfo[]) + 0x56 at System.Reflection.Runtime.General.TypeUnifier.GetConstructedGenericType(RuntimeTypeInfo, RuntimeTypeInfo[]) + 0x2a at System.Reflection.Runtime.General.TypeResolver.TryResolveTypeSignature(TypeSpecificationHandle, MetadataReader, TypeContext, Exception&) + 0x945 at System.Reflection.Runtime.General.TypeResolver.TryResolve(Handle, MetadataReader, TypeContext, Exception&) + 0x160 at System.Reflection.Runtime.General.TypeResolver.Resolve(Handle, MetadataReader, TypeContext) + 0x61 at System.Reflection.Runtime.FieldInfos.NativeFormat.NativeFormatRuntimeFieldInfo.get_FieldRuntimeType() + 0x93 at System.Reflection.Runtime.FieldInfos.RuntimeFieldInfo.get_FieldType() + 0x70 at <Program>$.<Main>$(String[]) + 0x130

The compiler doesn't try to go out of its way to make sure we can access the field type either. Maybe that's fine?

I do not have opinion, except that consistency and less special cases is good.

Okay, let's see if we can get away without it. We can always restore that behavior.

jkotas

Thank you!

MichalStrehovsky · 2021-02-02T18:45:55Z

Of course I wrote a test for this exact scenario.

Looks like the exception message leaves things to be desired, so I need to fix that first to make this change acceptable.

This is a different take on dotnet#620. We want to make sure `MakeArrayType`/`MakeGenericType` fails if we don't have an EEType for the constructed type because that's the problematic API and it's good UX to have it fail there (as opposed to failing later because e.g. we can't get a `TypeHandle` in a `CreateInstance` call). But it's really inconvenient to have this fail e.g. when we're hydrating types for the purpose of comparing method signatures. I've seen a failure yesterday where we couldn't find the `string..ctor(char[])` overload because we didn't have an EEType for `ReadOnlySpan<char>` (in a signature of an unrelated overload). The AOT compiler could create those EETypes, but it's very wasteful. Instead, I'm relaxing the EEType check so that we only do it from the public APIs. It means people can get to "unusable" `RuntimeTypes` now if they're e.g. field types, or parameter types, but I believe those will mostly be corner cases. I'm adding tests to make sure all of this is diagnosable.

This is a different take on #620. We want to make sure `MakeArrayType`/`MakeGenericType` fails if we don't have an EEType for the constructed type because that's the problematic API and it's good UX to have it fail there (as opposed to failing later because e.g. we can't get a `TypeHandle` in a `CreateInstance` call). But it's really inconvenient to have this fail e.g. when we're hydrating types for the purpose of comparing method signatures. I've seen a failure yesterday where we couldn't find the `string..ctor(char[])` overload because we didn't have an EEType for `ReadOnlySpan<char>` (in a signature of an unrelated overload). The AOT compiler could create those EETypes, but it's very wasteful. Instead, I'm relaxing the EEType check so that we only do it from the public APIs. It means people can get to "unusable" `RuntimeTypes` now if they're e.g. field types, or parameter types, but I believe those will mostly be corner cases. I'm adding tests to make sure all of this is diagnosable.

MichalStrehovsky added the area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation label Feb 1, 2021

MichalStrehovsky requested a review from jkotas February 1, 2021 14:55

jkotas reviewed Feb 2, 2021

View reviewed changes

jkotas approved these changes Feb 2, 2021

View reviewed changes

Update CustomAttributeBasedDependencyAlgorithm.cs

c99a8d0

Fix diagnostic experience and test

09004c3

MichalStrehovsky merged commit 65941ce into dotnet:feature/NativeAOT Feb 5, 2021

MichalStrehovsky deleted the noConstructedTypeFromAttribute branch February 5, 2021 09:47

MichalStrehovsky mentioned this pull request Apr 15, 2021

Allow RuntimeType with missing EEType in some cases #965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit impact of `typeof()` in a custom attribute #620

Limit impact of `typeof()` in a custom attribute #620

MichalStrehovsky commented Feb 1, 2021 •

edited

Loading

jkotas commented Feb 1, 2021

MichalStrehovsky commented Feb 1, 2021

jkotas Feb 2, 2021

MichalStrehovsky Feb 2, 2021

MichalStrehovsky Feb 2, 2021

jkotas Feb 2, 2021

MichalStrehovsky Feb 2, 2021

jkotas left a comment

MichalStrehovsky commented Feb 2, 2021

	private static void ValidateElementType(RuntimeTypeInfo elementType, RuntimeTypeHandle typeHandle, bool multiDim, int rank)
	{
	Debug.Assert(multiDim \|\| rank == 1);

	if (elementType.IsByRef)
	throw new TypeLoadException(SR.Format(SR.ArgumentException_InvalidArrayElementType, elementType));

	// We only permit creating parameterized types if the pay-for-play policy specifically allows them or if the result
	// type would be an open type.
	if (typeHandle.IsNull() && !elementType.ContainsGenericParameters && !(elementType is RuntimeCLSIDTypeInfo))
	throw ReflectionCoreExecution.ExecutionDomain.CreateMissingArrayTypeException(elementType, multiDim, rank);
	}

	protected sealed override RuntimeConstructedGenericTypeInfo Factory(UnificationKey key)
	{
	bool atLeastOneOpenType = false;
	foreach (RuntimeTypeInfo genericTypeArgument in key.GenericTypeArguments)
	{
	if (genericTypeArgument.IsByRef \|\| genericTypeArgument.IsGenericTypeDefinition)
	throw new ArgumentException(SR.Format(SR.ArgumentException_InvalidTypeArgument, genericTypeArgument));
	if (genericTypeArgument.ContainsGenericParameters)
	atLeastOneOpenType = true;
	}

	// We only permit creating parameterized types if the pay-for-play policy specifically allows them or if the result
	// type would be an open type.
	if (key.TypeHandle.IsNull() && !atLeastOneOpenType)
	throw ReflectionCoreExecution.ExecutionDomain.CreateMissingConstructedGenericTypeException(key.GenericTypeDefinition, key.GenericTypeArguments.CloneTypeArray());

	return new RuntimeConstructedGenericTypeInfo(key);
	}

Limit impact of typeof() in a custom attribute #620

Limit impact of typeof() in a custom attribute #620

Conversation

MichalStrehovsky commented Feb 1, 2021 • edited Loading

jkotas commented Feb 1, 2021

MichalStrehovsky commented Feb 1, 2021

jkotas Feb 2, 2021

Choose a reason for hiding this comment

MichalStrehovsky Feb 2, 2021

Choose a reason for hiding this comment

MichalStrehovsky Feb 2, 2021

Choose a reason for hiding this comment

jkotas Feb 2, 2021

Choose a reason for hiding this comment

MichalStrehovsky Feb 2, 2021

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

MichalStrehovsky commented Feb 2, 2021

Limit impact of `typeof()` in a custom attribute #620

Limit impact of `typeof()` in a custom attribute #620

MichalStrehovsky commented Feb 1, 2021 •

edited

Loading